Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add overflow-checking variant for primitive arithmetic kernels and explicitly define overflow behavior #2643

Merged
merged 11 commits into from
Sep 4, 2022

Conversation

viirya
Copy link
Member

@viirya viirya commented Sep 3, 2022

Which issue does this PR close?

Closes #2641.
Closes #2642.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the arrow Changes to the arrow crate label Sep 3, 2022
@viirya viirya changed the title Add overflow-checking variant for add kernel and explicitly define overflow behavior for add Add overflow-checking variant for primitive arithmetic kernels and explicitly define overflow behavior Sep 3, 2022
@viirya viirya added the api-change Changes to the arrow API label Sep 3, 2022
arrow/src/datatypes/native.rs Outdated Show resolved Hide resolved
}

/// This is similar to `math_checked_op` but just for divide op.
fn math_checked_divide<LT, RT, F>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between this function and math_checked_divide_op and why do we need both of them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally I hope we can just have one. Currently math_checked_divide_op is used by divide_dyn and I want to limit the range of change to primitive kernels only.

@@ -1042,15 +1196,18 @@ pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
/// Perform `left / right` operation on two arrays without checking for division by zero.
/// The result of dividing by zero follows normal floating point rules.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update the doc

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean "The result of dividing by zero follows normal floating point rules"? I think this is not changed? It will panic as usual.

Copy link
Contributor

@HaoYang670 HaoYang670 Sep 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean "The result of dividing by zero follows normal floating point rules"?

Yes. But why follows normal floating point rules here ? It seems like the function has supported other numeric types. (T: datatypes::ArrowNumericType)

I think this is not changed? It will panic as usual.

Nope, but float will never panic. Divide by zero in float type gives inf or nan. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=972d301e807f9a6cfd2ba644b763b86c

Maybe it is better to doc the different behaviour between float and other types

for float, dividing by zero follows the normal floating point rules,
for other numeric types, dividing be zero will panic,
...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. Yea, let me update the doc.

@tustvold
Copy link
Contributor

tustvold commented Sep 3, 2022

I'm not a massive fan of forcing users to choose between slow but correct or fast but may have inconsistent behaviour, especially as having parallel kernels increases the likelihood of further divergent behaviour...

Taking a step back I wonder if we could just define the overflow behaviour as wrapping, and use explicit wrapping_op to avoid signed overflow panics in non-release builds. This avoids runtime penalties, is consistent with how Rust handles overflow (unlike C++ signed integer overflow is actually defined, the debug panics are just "helpful"), and is what I at least would expect to occur.

I'm not sure what SQL says on the topic of overflow, if anything, which may be relevant here? Perhaps @alamb knows?

@viirya
Copy link
Member Author

viirya commented Sep 3, 2022

I'm not a massive fan of forcing users to choose between slow but correct or fast but may have inconsistent behaviour, especially as having parallel kernels increases the likelihood of further divergent behaviour...

I think that you're talking about divide_checked. Another thought is, I guess the non-simd one should be optimized by the compiler? Not sure how much performance difference between them.

I was thinking if possibly to do same thing on simd_checked_divide_op. But seems simd integers (packed_simd2) don't provide similar wrapping/checked APIs.

Taking a step back I wonder if we could just define the overflow behaviour as wrapping, and use explicit wrapping_op to avoid signed overflow panics in non-release builds. This avoids runtime penalties, is consistent with how Rust handles overflow (unlike C++ signed integer overflow is actually defined, the debug panics are just "helpful"), and is what I at least would expect to occur.

Hmm, is that something we want to have? Actually it may cause more difficulty for us to use this crate. As I mentioned below, we actually need two variants: overflow-checking (currently it could be by setting overflow-checks cargo flag) and overflow-as-null. I don't think defining the overflow behavior as wrapping is good idea. It sounds like a regression from current status. Users cannot choose overflow-checking behavior after that.

I'm not sure what SQL says on the topic of overflow, if anything, which may be relevant here? Perhaps @alamb knows?

This is the next think we want to do. Actually it is more important to us. In Spark, once configured, it is allowed to have overflow. Overflowing value will be represented as NULL.

That's being said, we can skip this change (overflow-checking variant/non-overflow-checking variant) if it cannot reach consensus. I just thought to have overflow/non-overflow variants like C++ is a good idea.

We actually need an overflow-checking variant and an overflow-as-null variant. And the current arithmetic kernels are overflow-checking variant already (if overflow-checks is enabled by users). We just need to add an overflow-as-null variant.

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies I misread what this PR is doing. The important thing for me is the default kernels do not perform expensive overflow checking.

Having checked variants that return an error makes sense to me

Edit: It occurs to me that we still need to update the dyn kernels and the scalar kernels to be consistent, potentially adding checked variants, but that can easily be done as a follow up

Ok(Some(r))
} else {
// Overflow
Err(ArrowError::ComputeError("Overflow happened".to_string()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could print the problematic values

Err(ArrowError::ComputeError("DivideByZero".to_string()))
} else {
// Overflow
Err(ArrowError::ComputeError("Overflow happened".to_string()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

T::Native: Add<Output = T::Native>,
T::Native: ArrowNativeTypeOp,
{
math_op(left, right, |a, b| a.wrapping_add_if_applied(b))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

macro_rules! native_type_op {
($t:tt) => {
impl ArrowNativeTypeOp for $t {
fn checked_add_if_applied(self, rhs: Self) -> Option<Self> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the if_applied suffix? Is it because wrapping operations are not applicable to floating point types? Perhaps we could call them add and add_checked to match the kernels?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can just have one API name here as we cannot distinguish what native type we are calling from the kernels. That's why I make such API name. :)

I can rename it to add_checked and add some comments to say for floating point types, it is simply add without check.

@@ -114,6 +115,98 @@ pub trait ArrowPrimitiveType: 'static {
}
}

/// Trait for ArrowNativeType to provide overflow-aware operations.
pub trait ArrowNativeTypeOp:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be public?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I think. I will remove pub in next commit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it has to be, as it is used as type bound in public APIs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use a trick of putting the trait in a private module, up to you

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Sounds good. Let me update it.

pub trait ArrowNativeTypeOp:
ArrowNativeType
+ Add<Output = Self>
+ Sub<Output = Self>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the default impls a bit confusing here, it took me a bit to realise they're only used by the floating point types

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I will add some comments.

///
/// This detects overflow and returns an `Err` for that. For an non-overflow-checking variant,
/// use `multiply` instead.
pub fn multiply_check<T>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn multiply_check<T>(
pub fn multiply_checked<T>(

/// The APIs with `wrapping` suffix are the variant of non-overflow-checking. If overflow
/// occurred, they will supposedly wrap around the boundary of the type.
///
/// The APIs with `_check` suffix are the variant of overflow-checking which return `None`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_checked?

@@ -1013,18 +1171,21 @@ where
/// Perform `left / right` operation on two arrays. If either left or right value is null
/// then the result is also null. If any right hand value is zero then the result of this
/// operation will be `Err(ArrowError::DivideByZero)`.
pub fn divide<T>(
///
/// When `simd` feature is not enabled. This detects overflow and returns an `Err` for that.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when SIMD is enabled?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got signal: 8, SIGFPE: erroneous arithmetic operation. This is original behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, that would imply rust division is always checked 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup - and LLVM cannot vectorize it correctly - https://rust.godbolt.org/z/T8eTGM8zn

@@ -1040,17 +1201,21 @@ pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
}

/// Perform `left / right` operation on two arrays without checking for division by zero.
/// The result of dividing by zero follows normal floating point rules.
/// For floating point types, the result of dividing by zero follows normal floating point
/// rules. For other numeric types, dividing by zero will panic,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems inconsistent with the other APIs, perhaps it should saturate instead

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, saturating_div also panics when dividing by zero.

@tustvold tustvold merged commit 6d86472 into apache:master Sep 4, 2022
@ursabot
Copy link

ursabot commented Sep 4, 2022

Benchmark runs are scheduled for baseline = 4c1bb00 and contender = 6d86472. 6d86472 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@viirya
Copy link
Member Author

viirya commented Sep 4, 2022

Thanks for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API arrow Changes to the arrow crate
Projects
None yet
4 participants